Update Protobuf Bazel Workspace#7094
Update Protobuf Bazel Workspace#7094psamanoelton wants to merge 22 commits intotensorflow:masterfrom
Conversation
arcra
left a comment
There was a problem hiding this comment.
As discussed offline, I'd like some optional and somewhat independent changes to be done in separate PRs, ideally.
Also, not sure what issues you were finding with the urllib and six packages, but there were a couple related PRs: #7016 and #7013 (we vendor the urllib3 package, IIRC, and according to that second PR, the six library shouldn't be necessary anymore).
| sudo apt-get update | ||
| sudo apt-get install -y libgbm-dev libxss1 libasound2 | ||
| sudo apt-get install -y "${py_abi}-dev" libgbm-dev libxss1 libasound2 | ||
| # Bazel's system_python repository resolves headers relative to the |
There was a problem hiding this comment.
Is this a problem that started from upgrading to bazel version 7.7? Or from running in our self-hosted container? Or with the new protobuf version?
Perhaps this comment explains it... although it's not super clear to me either. Or maybe an incompatibility of the setup-python action in a self-hosted runner, as described here.
Generally, this seems hacky for something that I expect should be easily supported (setting up python).
Having said this, I think the environment is fairly complex, so if this works, I guess I'm ok with it, but it will just be complex to maintain. If we could make it work in a simpler way, that would be my preference.
Perhaps we can try either:
- Using the env var mentioned in the "advance usage" documentation of the setup-python action (linked above)
- Using a newer setup-python that might handle this better
- Remove the setup-python action, and just installing the specific python version via shell commands directly in the container.
We can leave this as a follow-up, but if possible, I'd like this change to be submitted as a standalone PR.
| # in our WORKSPACE file. | ||
| BAZEL_VERSION: '6.5.0' | ||
| BAZEL_SHA256SUM: 'a40ac69263440761199fcb8da47ad4e3f328cbe79ffbf4ecc14e5ba252857307' | ||
| BAZEL_VERSION: '7.7.0' |
There was a problem hiding this comment.
FWIW, my suggestion to upgrade Bazel version to 7.7 was to use bazel modules, which should ideally help with dependency management.
Are we getting any benefit from doing this upgrade? Does it help updating some dependencies or something?
| def protobuf_java_export(**kwargs): | ||
| java_export( | ||
| javacopts = JAVA_RELEASE_OPTS, | ||
| - # https://github.com/bazelbuild/rules_jvm_external/issues/1245 |
There was a problem hiding this comment.
Can we add comments for why we are patching these changes? (all of these files)
| @@ -2,6 +2,11 @@ | |||
|
|
|||
| We use [patch-package](https://www.npmjs.com/package/patch-package) to apply | |||
There was a problem hiding this comment.
Is this still accurate? (i.e. are we still using patch-package somewhere?) Or should we remove this comment?
It's unclear with the current phrasing. If this is still used, can you rephrase to make it clear that both methods are used for different libraries? (and on what this depends or why we're using two methods?)
| srcs = ["__init__.py"], | ||
| srcs_version = "PY3", | ||
| # Keep the package's real __init__.py on this target instead of depending | ||
| # on :compat so Bazel 7 test runfiles expose the lazy tf/tf2 exports from |
There was a problem hiding this comment.
Is this bazel-7-specific? We might forget to update this if we update the project to use a new bazel version. Could we just say "bazel", without the specific version?
| path = "third_party/safe_html_types", | ||
| ) | ||
|
|
||
| # rules_closure's Soy toolchain still expects safe-html-types classes that are |
There was a problem hiding this comment.
Can this comment be above the previous lines where the local repository is defined?
| @@ -53,19 +84,108 @@ py_repositories() | |||
|
|
|||
| http_archive( | |||
There was a problem hiding this comment.
I thought you'd use the tb_http_archive rule here. Why is it only used in some places?
| ) | ||
|
|
||
| java_import_external( | ||
| name = "com_google_template_soy", |
There was a problem hiding this comment.
Can you add comments for why we added all of these dependencies? What needs this template soy package? And why are we adding the extra_build_file_content?
| package_json_remove = ["scripts.postinstall"], | ||
| patch_args = ["-p1"], | ||
| # Under Bazel 7.7.0 on this stack, invoking patch-package from the | ||
| # repository rule is fragile. Apply the existing git-format patches |
There was a problem hiding this comment.
What does "fragile" mean in this context? It would sometimes not work correctly?
| omit_com_google_common_html_types = True, | ||
| omit_com_google_protobuf = True, | ||
| omit_com_google_protobuf_js = True, | ||
| omit_com_google_template_soy = True, |
There was a problem hiding this comment.
Does this mean the rules_closure_dependencies are the ones that require the new dependencies above?
TensorBoard 2.21: Bazel 7.7.0 + protobuf 6.31.1 upgrade
Summary
This PR upgrades the local TensorBoard
2.21branch to:7.7.06.31.11.74.0The goal is to align TensorBoard's local/source build more closely with TensorFlow
2.21while preserving:bazel test //tensorboard/...What changed
1. Bazel 7.7.0 migration
7.7.0.6.xto7.7.0+.--noenable_bzlmod--incompatible_default_to_explicit_init_py2. protobuf 6.31.1 alignment
6.31.1.1.74.0.py_proto_libraryandpy_grpc_libraryinstead of older protobuf internals removed in newer protobuf/rules stacks.3. WORKSPACE / external repo compatibility
tb_http_archivehelper in third_party/repo.bzl for mirrored downloads and patch application.rules_ccrules_javaabsl4. rules_closure / Soy / Java compatibility
rules_closureCLI usage in patches/rules_closure_soy_cli.patch.5. TensorBoard compat / Bazel Python behavior
importlib.import_module(...)for safer loading.__init__.pyexplicitly.6. Test and runtime environment hardening
7. Plugin summary import fixes
tf2resolution inno_tensorflowpaths:8. Miscellaneous compatibility fixes
rules_webtesting.Validation
Validated locally in Docker against a fresh clone of the
protobufbranch.Commands used:
Additional manual runtime check:
Important notes for reviewers
Why
test --incompatible_default_to_explicit_init_pyis test-onlyUsing this flag globally fixed Bazel test runfiles but broke wheel packaging by changing the package tree copied into the pip wheel build. Scoping it to
testkeeps:test_pip_packagegreenWhy
third_party/safe_html_typesis vendoredThis is the largest part of the diff. It is included because the older Closure/Soy stack used by TensorBoard expects safe-html-types classes that remain compatible with protobuf-java 6.x. Without this vendored copy, the Bazel 7 / protobuf 6 migration fails later in Java/Soy tooling rather than in TensorBoard code directly.
Follow-up ideas
These are not required for this PR, but may be worth considering later:
6.31.1instead of allowing newer6.x/7.x.WORKSPACEcompatibility wiring into more focusedthird_partyhelpers.Final status
bazel query //...: passingupdate_protos_test: passingbazel test //tensorboard/...: passing